Skip to content

Remove FSA check in handleAction(s)#168

Merged
timche merged 1 commit intomasterfrom
fix/handle-non-fsa
Nov 21, 2016
Merged

Remove FSA check in handleAction(s)#168
timche merged 1 commit intomasterfrom
fix/handle-non-fsa

Conversation

@timche
Copy link
Copy Markdown
Member

@timche timche commented Nov 21, 2016

With #141 we have made other libraries incompatible with redux-actions and it resulted to an unexpected behavior (#164, #167). If an action is not a FSA, handleAction should not handle it and just pass it to the next reducer. This fix will remove the FSA check to support Non-FSA.

Closes #164, #167

@timche timche force-pushed the fix/handle-non-fsa branch 2 times, most recently from 12d5542 to 9da590b Compare November 21, 2016 19:17
Copy link
Copy Markdown
Contributor

@globalchubby globalchubby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does there need to be a change in the README as well?

Comment thread src/handleAction.js
'The FSA spec mandates an action object with a type. Try using the createAction(s) method.'
);

if (!includes(actionTypes, action.type.toString())) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const {type: actionType} = action

or

const {type} = action

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it.

@timche
Copy link
Copy Markdown
Member Author

timche commented Nov 21, 2016

We didn't made any changes when we added the check. The current README makes it pretty clear that handleAction only handles FSA.

}
})).to.deep.equal({
counter: 0
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a chai assertion that we expect this code block not to throw? I think that's the point, and not that the state is reduced to a certain value.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reducer should not throw AND just return the state. Will change that.

With #141 we have made other libraries incompatible with redux-actions and it resulted to an unexpected behavior (#164, #167). If an action is not a FSA, handleAction should not handle it and just pass it to the next reducer. This fix will remove the FSA check to support Non-FSA.

Closes #164, #167
@timche timche force-pushed the fix/handle-non-fsa branch from afb556f to ef4acb7 Compare November 21, 2016 19:29
@timche
Copy link
Copy Markdown
Member Author

timche commented Nov 21, 2016

Please review again @yangmillstheory.

@globalchubby
Copy link
Copy Markdown
Contributor

Just have one last question: #168 (comment)

@timche
Copy link
Copy Markdown
Member Author

timche commented Nov 21, 2016

Already answered and added :P

@globalchubby
Copy link
Copy Markdown
Contributor

Was this test failing before the application code changed?

@timche timche merged commit ef4acb7 into master Nov 21, 2016
@timche
Copy link
Copy Markdown
Member Author

timche commented Nov 21, 2016

What do you mean exactly?

@timche timche deleted the fix/handle-non-fsa branch November 21, 2016 20:02
@globalchubby
Copy link
Copy Markdown
Contributor

If we removed the FSA check removal, then the test should fail (TDD) to eliminate false positives. Just wondering if that's the case

@timche
Copy link
Copy Markdown
Member Author

timche commented Nov 21, 2016

The test I've wrote was failing when I've just removed the checks, because type was missing.

@globalchubby
Copy link
Copy Markdown
Contributor

Great, thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Non-FSA actions cause handleAction reudcers to emit confusing error messages

2 participants